Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cancellation methods for the API #4

Merged
merged 7 commits into from
Jul 27, 2023
Merged

Cancellation methods for the API #4

merged 7 commits into from
Jul 27, 2023

Conversation

denisonbarbosa
Copy link
Member

@denisonbarbosa denisonbarbosa commented Jul 25, 2023

Our current implementation lacks methods for aborting the session and canceling the IsAuthorized blocking operation. This PR adds those methods and propagates the necessary changes to the service files and the current brokers.

UDENG-1107

The GetAuthenticationModes function modifies the receiver, so its
function signature should use a *exampleBroker to indicate that it does
modify the receiver.
The broker can handle multiple requests at once, even for the same
username, so we need some write safety when doing changes on the broker
such as adding/deleting keys from the currentSessions map
@denisonbarbosa denisonbarbosa marked this pull request as ready for review July 25, 2023 16:45
@denisonbarbosa denisonbarbosa requested a review from a team as a code owner July 25, 2023 16:45
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

80% there, no bad for a first shot :)

Please have a look at my feedbacks in details, as some points needs to be reworked (on the cancellation phase) and some needs some exploration. Otherwise, the base idea is here!

internal/brokers/examplebroker.go Outdated Show resolved Hide resolved
internal/brokers/examplebroker.go Show resolved Hide resolved
internal/brokers/manager.go Outdated Show resolved Hide resolved
internal/services/pam/pam.go Show resolved Hide resolved
internal/services/pam/pam.go Outdated Show resolved Hide resolved
pam/pam.go Outdated Show resolved Hide resolved
pam/pam.go Outdated Show resolved Hide resolved
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two smalls and one missing piece, otherwise good! Close to get merged :)

internal/brokers/examplebroker.go Outdated Show resolved Hide resolved
pam/pam.go Outdated Show resolved Hide resolved
internal/brokers/broker.go Outdated Show resolved Hide resolved
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments on the last changes. Then, I think we will be good to go!

internal/brokers/broker.go Outdated Show resolved Hide resolved
internal/brokers/broker.go Outdated Show resolved Hide resolved
internal/brokers/responses.go Outdated Show resolved Hide resolved
pam/pam.go Show resolved Hide resolved
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could have done me a favor and only add a commit for the finale review!

Of course, despite holding you forever highly accountable for it, let me approve this :) Feel free to rebase/arrange the commit and merge, nice work!

We were using static strings for handling the broker responses and this
was prone to error and would complicate future changes, so those values
should be well-defined somewhere to avoid those obstacles
This adds both AbortSession and CancelIsAuthorized to the Brokerer
interface. Those methods allow us to handle cancellation of the calls
through communication with the actual broker, instead of solely relying
on context cancellation as it is not possible to do it through dbus.
This is an operation that we do constantly, so it deserves its own
function.
Now that we have a proper cancellation API, this updates the pam
module with the new way of interacting with those calls.
AbortSession is now an exported method that can be used through the
client
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants